Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed symbol declarations for generic filtering mapped types #53207

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Fixed symbol declarations for generic filtering mapped types #53207

merged 2 commits into from
Mar 15, 2023

Conversation

Andarist
Copy link
Contributor

the issue was noticed here

this PR is basically a fix for an unnoticed bug in the feature implemented in #51650 , the implementation itself is just a pulled-out refactor from #52972 since it (accidentally) fixed this issue

cc @sandersn @weswigham (reviewers of my original PR)

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 11, 2023
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Andarist
Andarist commented Mar 11, 2023
View reviewed changes
src/compiler/checker.ts
@@ -13174,8 +13174,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// and T as the template type.
const typeParameter = getTypeParameterFromMappedType(type);
const constraintType = getConstraintTypeFromMappedType(type);
const nameType = getNameTypeFromMappedType(type.target as MappedType || type);
const isFilteringMappedType = nameType && isTypeAssignableTo(nameType, typeParameter);
Copy link
Contributor Author

@Andarist Andarist Mar 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bug was here, this likely should have been something like this:

-const isFilteringMappedType = nameType && isTypeAssignableTo(nameType, typeParameter);
+const isFilteringMappedType = nameType && isTypeAssignableTo(nameType, getTypeParameterFromMappedType(type.target as MappedType || type));

and this is what happens in the added isFilteringMappedType since it unpacks the correct mapped type on its own and uses it as the source of information for both arguments passed to isTypeAssignableTo

@jakebailey
Copy link
Member

Is this something we might want to cherry-pick into a 5.0 patch?

@Andarist
Copy link
Contributor Author

Andarist commented Mar 13, 2023 •
edited
Loading

I would love for it to happen. The original intention was to make this work - it's just a silly bug that slipped our eyes.

weswigham
weswigham requested changes Mar 14, 2023
View reviewed changes
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a fine fix. I have a small style nit, since you extracted some stuff into a constant anyway. TBD what patch we'll pull this in to, but probably a 5.0.3, since this is a bit late for 5.0.2.

src/compiler/checker.ts
const isFilteringMappedType = nameType && isTypeAssignableTo(nameType, typeParameter);
const mappedType = (type.target as MappedType) || type;
const nameType = getNameTypeFromMappedType(mappedType);
const shouldLinkPropDeclarations = !nameType || isFilteringMappedType(mappedType);
const templateType = getTemplateTypeFromMappedType(type.target as MappedType || type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should also just reference mappedType now.

@Andarist Andarist requested review from weswigham and removed request for sandersn and jakebailey March 15, 2023 00:54
@weswigham weswigham merged commit eb6aaea into microsoft:main Mar 15, 2023
@Andarist
Copy link
Contributor Author

@weswigham @jakebailey what’s the process of cherry-picking this into 5.0.3? Is this something that you will consider for this fix?

@jakebailey
Copy link
Member

jakebailey commented Mar 15, 2023 •
edited
Loading

The process is:

@typescript-bot cherry-pick this to release-5.0

And we can discuss / merge it once 5.0.2 is out.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 15, 2023 •
edited
Loading

Heya @jakebailey, I've started to run the task to cherry-pick this into release-5.0 on this PR at 681f8bc. You can monitor the build here.

@jakebailey jakebailey added this to the TypeScript 5.0.3 milestone Mar 15, 2023
@typescript-bot typescript-bot mentioned this pull request Mar 15, 2023
Merged
@typescript-bot
Copy link
Collaborator

Hey @jakebailey, I've opened #53271 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Mar 15, 2023
@Andarist @typescript-bot
Cherry-pick PR microsoft#53207 into release-5.0
52af128 
Component commits:
dab0d38 Fixed symbol declarations for generic filtering mapped types

681f8bc reuse the added mappedType variable in one more place
DanielRosenwasser pushed a commit that referenced this pull request Mar 28, 2023
@Andarist
🤖 Pick PR #53207 (Fixed symbol declarations for gener...) into releas…
111df34 
…e-5.0 (#53271)

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
@Andarist Andarist mentioned this pull request May 3, 2023
Closed
drivron pushed a commit to scenari/typescript that referenced this pull request Sep 14, 2023
@Andarist
🤖 Pick PR microsoft#53207 (Fixed symbol declarations for gener...) in…
d9ef7d9 
…to release-5.0 (microsoft#53271)

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weswigham weswigham weswigham approved these changes

Assignees
No one assigned
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Milestone
TypeScript 5.0.3
Development

Successfully merging this pull request may close these issues.
4 participants
@Andarist @typescript-bot @jakebailey @weswigham